Add some new "status quo" atime tests for common operations #4517
Conversation
|
Should also cover comment in #4666
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4517 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 81 81
Lines 15203 15203
=======================================
Hits 15017 15017
Misses 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
cc @Anirban166 this old draft PR has some potential new benchmarking tests for #6078. If we extract the good tests from here I think we could close this PR too. |
|
@tdhock what would the format be for pulling examples like this into {atime}, exactly? thus far all of our examples are like slow/fast pairs, but this is structured as standalone "status quo" tests we'd like to prevent from slowing in the future. Would it be appropriate to add them as tests with just one label? "Fast" or "Status quo" maybe? |
|
sure it is possible to omit the Slow and just include Fast (=Status Quo). |
Generated via commit 73efaf3 Download link for the artifact containing the test results: ↓ atime-results.zip
|
MichaelChirico
requested review from
Anirban166,
MichaelChirico and
tdhock
as code owners
|
@jangorecki I trimmed away a lot of what was here & just pulled out a few common operations that are ubiquitous to serve as baseline performance tests, PTAL. @tdhock I think since there's no reference to any commit hash in this branch, we can proceed to delete this branch after merging, right? |
MichaelChirico
changed the title
3 participants
After having discussion with Matt on slack we decided to narrow down scope of #4687. So new benchmarking feature can be more usable and not introduce extra maintenance burden that tracking historical timings, and other features initially listed here, would require.
As a starting point I took system.time tests that have been already taken out from tests.Rraw file (to reduce run time of main test script). Those tests have been moved to a new benchmark() function that meant to replace test() function when system.time is needed.
To keep things simple, we don't need new benchmark.data.table(), as we can just call
test.data.table("benchmarks.Rraw")orcc("benchmarks.Rraw")in dev-mode. It will already recognize benchmarks calls in the test script.This is still very much a starting point so any feedback is very welcome.
Ideas for improvement:
timesargument to run expression multiple times and take mean/median to compare. This will allow to make a more tight tolerance (test 1110).Initial proposal at bc8a8be
PR brings new set of scripts, and internal functions, to measure performance of data.table. They are not run in any of our workflow as of now, but rather should be run manually. For now there is no point to merge this branch. Opening PR to more easily document and refer to amongst gh issues.
For example, addressing "add timing test for many .SD cols #3797" for which scripts are defined in
benchmarks.Rrawfile. Yet to close #3797 we need to add a rules to be checked after all benchmarks, to confirm optimize=0 is not that much different than optimize=Inf.